[Build] fix file_data_loader.cpp build issues for windows by python3kgae · Pull Request #4899 · pytorch/executorch

python3kgae · 2024-08-26T03:53:35Z

Two Windows build issues have been addressed in this pull request:

Avoid closing when fd_ is -1, as this would cause a crash on Windows.
Introduce file.h, enabling Windows builds to utilize io.h and implement pread.

For #4661

pytorch-bot · 2024-08-26T03:53:38Z

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/4899

📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

⚠️ 3 Awaiting Approval

As of commit bc600ce with merge base a79b1a6 ():

AWAITING APPROVAL - The following workflows need approval before CI can run:

This comment was automatically generated by Dr. CI and updates every 15 minutes.

kirklandsign · 2024-08-26T18:51:51Z

Thank you @python3kgae !

I would prefer that we use a single file.h, and check for ifdef _WIN32. When we use #include please use the full path starting from executorch/... , not adding a include_dir in cmake. Or at least, using a file.h which selects the correct file_posix.h and file_windows.h based on compiler macro. We also maintain an internal build system and it's easier that way.

cc @dbort

Two build issues have been addressed in this pull request: 1. Avoid closing when fd_ is -1, as this would cause a crash on Windows. 2. Introduce separate headers for Windows and Unix, enabling Windows builds to utilize a distinct header and implement pread. For pytorch#4661

python3kgae · 2024-08-28T03:03:34Z

Thank you @python3kgae !

I would prefer that we use a single file.h, and check for ifdef _WIN32. When we use #include please use the full path starting from executorch/... , not adding a include_dir in cmake. Or at least, using a file.h which selects the correct file_posix.h and file_windows.h based on compiler macro. We also maintain an internal build system and it's easier that way.

cc @dbort

Changed into single file.h.

kirklandsign · 2024-08-28T17:22:48Z

Thank you! One more request: in extension/data_loader/targets.bzl line 43, can you add a line

headers = ["file.h"],

python3kgae · 2024-08-28T17:25:32Z

Thank you! One more request: in extension/data_loader/targets.bzl line 43, can you add a line
headers = ["file.h"],

Like this?

@@ -40,6 +40,7 @@ def define_common_targets():
     runtime.cxx_library(
         name = "file_data_loader",
         srcs = ["file_data_loader.cpp"],
+        headers = ["file.h"],
         exported_headers = ["file_data_loader.h"],
         visibility = [
             "//executorch/test/...",

kirklandsign · 2024-08-28T17:30:32Z

Thank you! One more request: in extension/data_loader/targets.bzl line 43, can you add a line
headers = ["file.h"],

Like this?

@@ -40,6 +40,7 @@ def define_common_targets():
     runtime.cxx_library(
         name = "file_data_loader",
         srcs = ["file_data_loader.cpp"],
+        headers = ["file.h"],
         exported_headers = ["file_data_loader.h"],
         visibility = [
             "//executorch/test/...",

Yes

python3kgae · 2024-08-28T17:49:26Z

Thank you! One more request: in extension/data_loader/targets.bzl line 43, can you add a line
headers = ["file.h"],

Like this?

@@ -40,6 +40,7 @@ def define_common_targets():
     runtime.cxx_library(
         name = "file_data_loader",
         srcs = ["file_data_loader.cpp"],
+        headers = ["file.h"],
         exported_headers = ["file_data_loader.h"],
         visibility = [
             "//executorch/test/...",

Yes

Done.

dbort

Normally we try to avoid using platform-based #ifdefs for implementations, and will create separate .cpp files that the build system brings in as necessary. But this is pretty light-weight and self-contained, so it seems fine.

dbort · 2024-08-29T23:07:05Z

extension/data_loader/file.h

@@ -0,0 +1,57 @@
+/*


Please call this file pread.h to make its purpose more clear.

dbort · 2024-08-29T23:07:55Z

extension/data_loader/file.h

+ * This source code is licensed under the BSD-style license found in the
+ * LICENSE file in the root directory of this source tree.
+ */
+


Please add a comment describing the purpose of this header: i.e., it ensures that a pread-compatible function is defined in the global namespace for windows and posix environments.

dbort · 2024-08-29T23:09:06Z

extension/data_loader/file.h

+
+#include <windows.h>
+
+inline ssize_t pread(int __fd, void* __buf, size_t __nbytes, size_t __offset) {


Please remove the leading underscores from the param names. Since this is our code and not part of the standard library, we shouldn't define any names beginning with underscore.

dbort · 2024-08-29T23:10:48Z

extension/data_loader/file.h

+  overlapped.Offset = __offset;
+  overlapped.OffsetHigh = __offset >> 32;


Will this do the right thing on both 32-bit and 64-bit architectures? Seems like it should, but I don't know enough about the definition of DWORD on different architectures

DWORD will always be 32 bits on both 32-bit and 64-bit Windows systems.

It's important to note that the nNumberOfBytesToRead parameter for ReadFile is a DWORD. Therefore, if we need to read more than 4GB of data at once, the current implementation needs to be updated to handle larger file sizes.

dbort · 2024-08-29T23:13:46Z

extension/data_loader/file.h

+// To avoid conflicts with std::numeric_limits<int32_t>::max() in
+// file_data_loader.cpp.
+#undef max


Please move this to the top of the file, closer to the header that defined it. If you know, please mention which header defined it, since on its own it's not clear why this file should bother undefining a macro that it didn't define.

shoumikhin · 2025-03-17T02:30:28Z

@tarun292, @kirklandsign can you take a look please?

github-actions · 2025-08-27T00:51:35Z

Looks like this PR hasn't been updated in a while so we're going to go ahead and mark this as Stale.
Feel free to remove the Stale label if you feel this was a mistake.
If you are unable to remove the Stale label please contact a maintainer in order to do so.
If you want the bot to never mark this PR stale again, add the no-stale label.
Stale pull requests will automatically be closed after 30 days of inactivity.

github-actions · 2025-10-27T00:51:35Z

Looks like this PR hasn't been updated in a while so we're going to go ahead and mark this as Stale.
Feel free to remove the Stale label if you feel this was a mistake.
If you are unable to remove the Stale label please contact a maintainer in order to do so.
If you want the bot to never mark this PR stale again, add the no-stale label.
Stale pull requests will automatically be closed after 30 days of inactivity.

github-actions · 2025-12-27T00:51:28Z

Looks like this PR hasn't been updated in a while so we're going to go ahead and mark this as Stale.
Feel free to remove the Stale label if you feel this was a mistake.
If you are unable to remove the Stale label please contact a maintainer in order to do so.
If you want the bot to never mark this PR stale again, add the no-stale label.
Stale pull requests will automatically be closed after 30 days of inactivity.

github-actions · 2026-02-25T00:58:25Z

Looks like this PR hasn't been updated in a while so we're going to go ahead and mark this as Stale.
Feel free to remove the Stale label if you feel this was a mistake.
If you are unable to remove the Stale label please contact a maintainer in order to do so.
If you want the bot to never mark this PR stale again, add the no-stale label.
Stale pull requests will automatically be closed after 30 days of inactivity.

Copilot

Pull request overview

This PR addresses native Windows build failures in the extension/data_loader file-backed loader by making fd cleanup safe on Windows and adding a Windows-compatible pread implementation.

Changes:

Add extension/data_loader/pread.h to provide a pread-compatible function on Windows (using Win32 APIs) while using the POSIX pread on non-Windows platforms.
Update FileDataLoader to include pread.h and remove the direct <unistd.h> dependency.
Avoid calling close() when fd_ == -1 in FileDataLoader’s destructor (moved-from safety on Windows).

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

File	Description
extension/data_loader/targets.bzl	Adds `pread.h` as a private header for the `file_data_loader` target.
extension/data_loader/pread.h	Introduces a cross-platform `pread` shim with a Windows implementation.
extension/data_loader/file_data_loader.cpp	Switches to the new `pread` shim and avoids closing `-1` fds.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-25T22:14:13Z

extension/data_loader/pread.h

+#include <executorch/runtime/platform/compiler.h> // For ssize_t.
+#include <io.h>
+
+#include <windows.h>
+// To avoid conflicts with std::numeric_limits<int32_t>::max() in
+// file_data_loader.cpp.
+#undef max
+
+inline ssize_t pread(int fd, void* buf, size_t nbytes, size_t offset) {
+  OVERLAPPED overlapped; /* The offset for ReadFile. */
+  memset(&overlapped, 0, sizeof(overlapped));


pread.h isn't self-contained on Windows: it uses memset, errno, and EIO but doesn't include headers that declare/define them. Since file_data_loader.cpp includes this header before <cstring>/<cerrno>, this can cause Windows compile errors. Add the appropriate standard headers (e.g., <cstring> and <cerrno>/<errno.h>).

Copilot · 2026-02-25T22:14:13Z

extension/data_loader/pread.h

+  overlapped.Offset = offset;
+  overlapped.OffsetHigh = offset >> 32;


overlapped.OffsetHigh = offset >> 32; can be undefined behavior on 32-bit builds because offset is a size_t (likely 32 bits) and shifting by 32 is UB. Consider first widening to uint64_t and then assigning the low/high 32-bit parts with explicit casts to DWORD to avoid UB and narrowing warnings.

Suggested change

overlapped.Offset = offset;

overlapped.OffsetHigh = offset >> 32;

ULONGLONG offset64 = (ULONGLONG)offset;

overlapped.Offset = (DWORD)offset64;

overlapped.OffsetHigh = (DWORD)(offset64 >> 32);

Copilot · 2026-02-25T22:14:13Z

extension/data_loader/pread.h

+  BOOL result; /* The result of ReadFile. */
+  DWORD bytes_read; /* The number of bytes read. */
+  HANDLE file = (HANDLE)_get_osfhandle(fd);
+
+  result = ReadFile(file, buf, nbytes, &bytes_read, &overlapped);
+  DWORD error = GetLastError();


ReadFile takes a DWORD byte count, but this wrapper passes size_t nbytes directly. On 64-bit this can silently truncate (and may warn/error under -Wconversion/-Werror). Add an explicit bounds check (e.g., nbytes <= MAXDWORD) and cast to DWORD before calling ReadFile; return -1 with an appropriate errno if the request is too large.

Copilot · 2026-02-25T22:14:14Z

extension/data_loader/pread.h

+        break;
+      default:
+        errno = EIO;
+        break;
+    }
+    return -1;


The ERROR_HANDLE_EOF case currently sets errno = 0 and returns -1. POSIX pread signals EOF by returning 0 (not an error). Returning -1 here can lead to misleading logs like strerror(0) ("Success") and incorrect error handling. Return 0 for this case to match pread semantics.

Suggested change

break;

default:

errno = EIO;

break;

}

return -1;

return 0;

default:

errno = EIO;

return -1;

}

Copilot · 2026-02-25T22:14:14Z

extension/data_loader/pread.h

+#include <windows.h>
+// To avoid conflicts with std::numeric_limits<int32_t>::max() in
+// file_data_loader.cpp.


On Windows, including <windows.h> without NOMINMAX typically defines both min and max macros. This header only #undef max, but file_data_loader.cpp uses std::min, which can be broken by the min macro. Consider defining NOMINMAX before including <windows.h> and/or #undef min alongside #undef max here.

Suggested change

#include <windows.h>

// To avoid conflicts with std::numeric_limits<int32_t>::max() in

// file_data_loader.cpp.

#ifndef NOMINMAX

#define NOMINMAX

#endif

#include <windows.h>

// To avoid conflicts with std::min/std::max and std::numeric_limits<int32_t>::max()

// in file_data_loader.cpp.

#undef min

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 26, 2024

python3kgae added 2 commits August 27, 2024 22:32

Use single file.h

be77d20

python3kgae force-pushed the win_bld_file branch from 54a7016 to be77d20 Compare August 28, 2024 03:00

kirklandsign requested a review from dbort August 28, 2024 17:22

Add file.h to targets.bzl

394f372

dbort requested changes Aug 29, 2024

View reviewed changes

Update per comment.

bc600ce

python3kgae marked this pull request as draft September 10, 2024 17:12

vortex-captain mentioned this pull request Dec 4, 2024

Windows x64 Build with support for xnnpack and llama example #6979

Closed

chinazhangchao mentioned this pull request Dec 6, 2024

Support build on native Windows x64 #7217

Closed

github-actions bot added the stale PRs inactive for over 60 days label Aug 27, 2025

kirklandsign requested a review from Copilot February 25, 2026 22:07

Copilot started reviewing on behalf of kirklandsign February 25, 2026 22:07 View session

Copilot AI reviewed Feb 25, 2026

View reviewed changes


		#include <windows.h>

		inline ssize_t pread(int __fd, void* __buf, size_t __nbytes, size_t __offset) {

		overlapped.Offset = __offset;
		overlapped.OffsetHigh = __offset >> 32;

		overlapped.Offset = offset;
		overlapped.OffsetHigh = offset >> 32;

-  overlapped.Offset = offset;
-  overlapped.OffsetHigh = offset >> 32;
+  ULONGLONG offset64 = (ULONGLONG)offset;
+  overlapped.Offset = (DWORD)offset64;
+  overlapped.OffsetHigh = (DWORD)(offset64 >> 32);

-#include <windows.h>
-// To avoid conflicts with std::numeric_limits<int32_t>::max() in
-// file_data_loader.cpp.
+#ifndef NOMINMAX
+#define NOMINMAX
+#endif
+#include <windows.h>
+// To avoid conflicts with std::min/std::max and std::numeric_limits<int32_t>::max()
+// in file_data_loader.cpp.
+#undef min

Conversation

python3kgae commented Aug 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pytorch-bot bot commented Aug 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/4899

⚠️ 3 Awaiting Approval

Uh oh!

kirklandsign commented Aug 26, 2024

Uh oh!

python3kgae commented Aug 28, 2024

Uh oh!

kirklandsign commented Aug 28, 2024

Uh oh!

python3kgae commented Aug 28, 2024

Uh oh!

kirklandsign commented Aug 28, 2024

Uh oh!

python3kgae commented Aug 28, 2024

Uh oh!

dbort left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shoumikhin commented Mar 17, 2025

Uh oh!

github-actions bot commented Aug 27, 2025

Uh oh!

github-actions bot commented Oct 27, 2025

Uh oh!

github-actions bot commented Dec 27, 2025

Uh oh!

github-actions bot commented Feb 25, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

python3kgae commented Aug 26, 2024 •

edited

Loading

pytorch-bot bot commented Aug 26, 2024 •

edited

Loading